Improve time complexity of removal of callback handle. #11751

chaoqin-li1123 · 2020-06-25T15:23:24Z

Commit Message: improve the time complexity of removal of a callback handle from O(N) to O(1) by storing a list iterator to itself inside the callback_holder. Currently, when a callback handle is removed from the list, the pointer of the handle is used for linear search to locate the callback holder and delete it. Since list.erase() cost constant time if we provide the iterator, and list iterator don't get invalidated in other list operations. We can store the iterator inside the callback holder when we initialize it and use it for constant time removal.
Additional Description:
Risk Level: Low
Testing: Pass all test in /test repo.
Docs Changes: No
Release Notes: No
[Optional Runtime guard:]
[Optional Fixes #Issue] #11665
[Optional Deprecated:]
Signed-off-by: chaoqinli chaoqinli@google.com

This PR proposes to rename the RetryPolicy field num_retries to max_retries. This parameter exists in two places: 1) the RetryPolicy message in the route configuration and 2) the header x-envoy-max-retries. The naming inconsistency is a UX papercut. max_retries feels like right name for what this field is for ie. the maximum number of retries that are permitted. There is also a stripped down RetryPolicy message which is used by RemoteDataSource which has a num_retries field. I'm including a matching rename of that for consistency. Risk Level: Low Testing: N/A Docs Changes: N/A Release Notes: N/A Signed-off-by: Martin Matusiak <numerodix@gmail.com> Signed-off-by: chaoqinli <chaoqinli@google.com>

repokitteh-read-only · 2020-06-25T15:35:02Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/envoy/.
CC @envoyproxy/api-watchers: FYI only for changes made to api/envoy/.

🐱

Caused by: #11751 was synchronize by chaoqin-li1123.

see: more, trace.

…nvoyproxy#11649) Commit Message: Split huge monolith mock header to speed up test compilation Additional Description: `cluster_manager_test` only used a simple mock class `MockAdmin` from `test/mocks/server/mocks.h`, which is a huge mock library. After splitting, the overall build time for `cluster_manager_test` reduced from 143.481s to 82.443s in my build cluster. Risk Level: low Testing: existing tests Docs Changes: N/A Release Notes: no Related Issues: envoyproxy#10917 Signed-off-by: Muge Chen <mugechen@google.com> Signed-off-by: chaoqinli <chaoqinli@google.com>

…nvoyproxy#11711) Risk Level: Low Testing: n/a Docs Changes: n/a Release Notes: n/a Signed-off-by: Alyssa Wilk <alyssar@chromium.org> Signed-off-by: chaoqinli <chaoqinli@google.com>

* Windows: test compilation refresh - Remove TestHeaderMapImplBase constructor that takes a list of std::pair<LowerCaseString, std::string> - MSVC erroneously treats this as ambiguous. It appears the explicit attribute of the LowerCaseString constructor is incorrectly discarded. We are able to reproduce this in a minimal example and see that MSVC is wrong here: https://godbolt.org/z/VjgsAi - To mitigate, remove LowerCaseString based constructor since it is only used in tests, tests always use std::string - Correct use of long in envoy_quic_alarm.cc, explicitly cast to int64_t (long on Windows is 32 bits) - `using std::string_literal::operator""s` is erroneously rejected by MSVC, throwing error C4455. - Instead, simply utilize the namespace and continue to use the operator as is - operator usage could be replaced by `std::string (const char* s, size_t n)` constructor - See https://developercommunity.visualstudio.com/content/problem/270349/warning-c4455-issued-when-using-standardized-liter.html and other related duplicate issues that have not yet been resolved. Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com> Co-authored-by: Sunjay Bhatia <sunjayb@vmware.com> Signed-off-by: William A Rowe Jr <wrowe@vmware.com> Co-authored-by: William A Rowe Jr <wrowe@vmware.com> Signed-off-by: chaoqinli <chaoqinli@google.com>

some cleanup of use of std container Signed-off-by: chaoqinli <chaoqinli@google.com>

Signed-off-by: chaoqinli <chaoqinli@google.com>

some cleanup of use of std container Signed-off-by: chaoqinli <chaoqinli@google.com>

Signed-off-by: chaoqinli <chaoqinli@google.com>

fault injection example did not work for delay option. After gdb debugging it was discovered that fault injection config requires delay part to be present. Without delay part Envoy assumes that Delay is disabled and does not scan RTDS for delay config updates. Risk Level: Low: Testing: Did manual testing as described in https://www.envoyproxy.io/docs/envoy/latest/start/sandboxes/fault_injection Docs Changes: No Release Notes: No Fixes: envoyproxy#11095 Signed-off-by: Christoph Pakulski <christoph@tetrate.io> Signed-off-by: chaoqinli <chaoqinli@google.com>

…tainer_cpp_cleanup

Signed-off-by: chaoqinli <chaoqinli@google.com>

…tainer_cpp_cleanup

Signed-off-by: chaoqinli <chaoqinli@google.com>

chaoqin-li1123 · 2020-06-27T23:58:07Z

There is a trade off between memory and performance here, each list iterator has a size of 8 bytes in 64 bit linux system. In stat_integration test of linux 64 release check, it is observed that about more 200 bytes will be consumed compared to the current version.

stevenzzzz

looks good.
A good start. LGTM.

stevenzzzz · 2020-06-29T14:32:13Z

test/integration/stats_integration_test.cc

@@ -286,7 +286,7 @@ TEST_P(ClusterMemoryTestRunner, MemoryLargeClusterSizeWithFakeSymbolTable) {
 // If you encounter a failure here, please see
 // https://github.com/envoyproxy/envoy/blob/master/source/docs/stats.md#stats-memory-tests
 // for details on how to fix.
- EXPECT_MEMORY_EQ(m_per_cluster, 44491);
+ EXPECT_MEMORY_EQ(m_per_cluster, 44715);
 EXPECT_MEMORY_LE(m_per_cluster, 46000); // Round up to allow platform variations.


could you update the comment section above and explain a little bit why this bump is necessary?

I have updated the comment. If there are a lot of callbacks in a callback manager or if adding and removing callbacks is frequent, making removal of callback handle O(1) will be a good trade off because storing a iterator is lightweight. It will make the memory consumption of a callback holder increase from 48 bytes to 56 bytes in a 64 bit linux system, but will be likely to improve performance when callback is used frequently.

stevenzzzz · 2020-06-29T15:07:05Z

/assign @htuch
PTAL chaoqin's warmup PR

…tainer_cpp_cleanup

Signed-off-by: chaoqinli <chaoqinli@google.com>

stevenzzzz · 2020-06-29T15:37:48Z

test/integration/stats_integration_test.cc

@@ -273,6 +273,7 @@ TEST_P(ClusterMemoryTestRunner, MemoryLargeClusterSizeWithFakeSymbolTable) {
 // 2020/05/13 10531 44425 44600 Refactor resource manager
 // 2020/05/20 11223 44491 44600 Add primary clusters tracking to cluster manager.
 // 2020/06/10 11561 44491 44811 Make upstreams pluggable
+ // 2020/06/29 11751 44715 46000 Improve time complexity of removing callback handle.


"... in callback manager."

yes, that will be more precise.

htuch

Seems reasonable, thanks!

htuch · 2020-06-29T23:25:03Z

source/common/common/callback_impl.h

@@ -46,24 +47,21 @@ template <typename... CallbackArgs> class CallbackManager {
 CallbackHolder(CallbackManager& parent, Callback cb) : parent_(parent), cb_(cb) {}

 // CallbackHandle
- void remove() override { parent_.remove(this); }
+ void remove() override { parent_.remove(it_); }


So, I think the safety of this depends on:

"Adding, removing and moving the elements within the list or across several lists does not invalidate the iterators or references. An iterator is invalidated only when the corresponding element is deleted."

in https://en.cppreference.com/w/cpp/container/list. I.e. that this is a std::list. I'd add some comments to the call sites here and where it_ is initialized, as well as to where callbacks_ is defined, to make sure that this is clear as an invariant.

Yes, it is guaranteed by C++ standard that list iterator won't be invalidated by list operation.

htuch · 2020-06-29T23:25:40Z

source/common/common/callback_impl.h

+
+ // the iterator of this callback holder inside callbacks_ list
+ // upon removal, use this iterator to delete callback holder in O(1)
+ typename std::list<CallbackHolder>::iterator it_;


I think what is done here is fine, as it's basically just std::list iterator semantics, which could be explained a bit better but are fairly clear.

htuch · 2020-06-29T23:26:23Z

source/common/router/scoped_rds.cc

- config_protos_(std::make_move_iterator(config_protos.begin()),
- std::make_move_iterator(config_protos.end())),
- rds_config_source_(std::move(rds_config_source)) {}
+ config_protos_(move(config_protos)), rds_config_source_(std::move(rds_config_source)) {}


Can you remove the unrelated optimization fixups and put them in a separate misc: PR?

Yes, I agree that these changes should be in another PR.

Signed-off-by: chaoqinli <chaoqinli@google.com>

…tainer_cpp_cleanup

Signed-off-by: chaoqinli <chaoqinli@google.com>

…tainer_cpp_cleanup

Signed-off-by: chaoqinli <chaoqinli@google.com>

…tainer_cpp_cleanup

…tainer_cpp_cleanup sync

htuch

LGTM, thanks!

) Improve the time complexity of removal of a callback handle from O(N) to O(1) by storing a list iterator to itself inside the callback_holder. Currently, when a callback handle is removed from the list, the pointer of the handle is used for linear search to locate the callback holder and delete it. Since list.erase() cost constant time if we provide the iterator, and list iterator don't get invalidated in other list operations. We can store the iterator inside the callback holder when we initialize it and use it for constant time removal. Risk Level: Low Testing: Pass all test in /test repo. Fixes envoyproxy#11665 Signed-off-by: chaoqinli <chaoqinli@google.com> Signed-off-by: scheler <santosh.cheler@appdynamics.com>

chaoqin-li1123 force-pushed the container_cpp_cleanup branch from 13fa9d2 to 20194d3 Compare June 25, 2020 15:34

chaoqin-li1123 requested review from alyssawilk, mattklein123, snowp and zuercher as code owners June 25, 2020 15:34

repokitteh-read-only bot added the api label Jun 25, 2020

chaoqin-li1123 force-pushed the container_cpp_cleanup branch 4 times, most recently from 8d52337 to c124adb Compare June 25, 2020 15:52

foreseeable and others added 8 commits June 25, 2020 16:10

build: removing hard-coded extension from generate_extension_db.py (e…

3eb115b

…nvoyproxy#11711) Risk Level: Low Testing: n/a Docs Changes: n/a Release Notes: n/a Signed-off-by: Alyssa Wilk <alyssar@chromium.org> Signed-off-by: chaoqinli <chaoqinli@google.com>

Signed-off-by: chaoqinli <chaoqinli@google.com>

e80b2a5

some cleanup of use of std container Signed-off-by: chaoqinli <chaoqinli@google.com>

make cast dynamic

dc16f65

Signed-off-by: chaoqinli <chaoqinli@google.com>

Signed-off-by: chaoqinli <chaoqinli@google.com>

e95fc7c

some cleanup of use of std container Signed-off-by: chaoqinli <chaoqinli@google.com>

make cast dynamic

89fc19f

Signed-off-by: chaoqinli <chaoqinli@google.com>

chaoqin-li1123 force-pushed the container_cpp_cleanup branch from c124adb to 2c9a3b8 Compare June 25, 2020 16:11

chaoqinli added 5 commits June 25, 2020 16:22

Merge branch 'master' of https://github.com/envoyproxy/envoy into con…

b03decc

…tainer_cpp_cleanup

format

eb5ed70

Signed-off-by: chaoqinli <chaoqinli@google.com>

format

1f211a3

Signed-off-by: chaoqinli <chaoqinli@google.com>

format

a2e7ffd

Signed-off-by: chaoqinli <chaoqinli@google.com>

Merge branch 'master' of https://github.com/envoyproxy/envoy into con…

72cc8ac

…tainer_cpp_cleanup

chaoqin-li1123 changed the title ~~Container cpp cleanup~~ Improve time complexity of removal of callback handle. Jun 25, 2020

chaoqinli added 3 commits June 26, 2020 01:20

Merge branch 'master' of https://github.com/envoyproxy/envoy into con…

d0354cb

…tainer_cpp_cleanup

rollback sth

2d9a9fa

Signed-off-by: chaoqinli <chaoqinli@google.com>

rollback sth

4c97b8d

Signed-off-by: chaoqinli <chaoqinli@google.com>

chaoqinli added 4 commits June 27, 2020 04:43

rollback sth

6dc2de4

Signed-off-by: chaoqinli <chaoqinli@google.com>

rollback sth

3e5ceaa

Signed-off-by: chaoqinli <chaoqinli@google.com>

increase memory upper bound

ccd18e6

Signed-off-by: chaoqinli <chaoqinli@google.com>

increase exact memory consumed

0d0a7d6

Signed-off-by: chaoqinli <chaoqinli@google.com>

stevenzzzz previously approved these changes Jun 29, 2020

View reviewed changes

repokitteh-read-only bot assigned htuch Jun 29, 2020

chaoqinli added 2 commits June 29, 2020 15:08

Merge branch 'master' of https://github.com/envoyproxy/envoy into con…

26de9e3

…tainer_cpp_cleanup

add comment of stats memory test

d75466a

Signed-off-by: chaoqinli <chaoqinli@google.com>

chaoqin-li1123 dismissed stevenzzzz’s stale review via d75466a June 29, 2020 15:21

stevenzzzz reviewed Jun 29, 2020

View reviewed changes

htuch reviewed Jun 29, 2020

View reviewed changes

chaoqinli added 9 commits June 30, 2020 02:09

add comments, remove unrelated fixups

a850f6d

Signed-off-by: chaoqinli <chaoqinli@google.com>

Merge branch 'master' of https://github.com/envoyproxy/envoy into con…

e8afadd

…tainer_cpp_cleanup

add comments, remove unrelated fixups

e227506

Signed-off-by: chaoqinli <chaoqinli@google.com>

fix format

7c62189

Signed-off-by: chaoqinli <chaoqinli@google.com>

fix format

d11b19a

Signed-off-by: chaoqinli <chaoqinli@google.com>

Merge branch 'master' of https://github.com/envoyproxy/envoy into con…

b1ba481

…tainer_cpp_cleanup

fix format

b2ce936

Signed-off-by: chaoqinli <chaoqinli@google.com>

Merge branch 'master' of https://github.com/envoyproxy/envoy into con…

fbc6a7d

…tainer_cpp_cleanup

Merge branch 'master' of https://github.com/envoyproxy/envoy into con…

85b84f4

…tainer_cpp_cleanup sync

chaoqin-li1123 requested a review from htuch July 6, 2020 22:11

htuch approved these changes Jul 7, 2020

View reviewed changes

htuch merged commit 4d4c585 into envoyproxy:master Jul 7, 2020

incfly mentioned this pull request Sep 21, 2020

Cherrypick callback manager improvement fix. istio/envoy#263

Merged

chaoqin-li1123 deleted the container_cpp_cleanup branch February 23, 2021 21:18

stevenzzzz mentioned this pull request Mar 16, 2021

~GrpcMuxWatchImpl() uses watches_.remove(this) which traverses the list. #15528

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve time complexity of removal of callback handle. #11751

Improve time complexity of removal of callback handle. #11751

chaoqin-li1123 commented Jun 25, 2020 •

edited

Loading

repokitteh-read-only bot commented Jun 25, 2020

chaoqin-li1123 commented Jun 27, 2020

stevenzzzz left a comment

stevenzzzz Jun 29, 2020

chaoqin-li1123 Jun 29, 2020

stevenzzzz commented Jun 29, 2020

stevenzzzz Jun 29, 2020

chaoqin-li1123 Jun 29, 2020

htuch left a comment

htuch Jun 29, 2020

chaoqin-li1123 Jun 29, 2020

htuch Jun 29, 2020

htuch Jun 29, 2020

chaoqin-li1123 Jun 29, 2020

htuch left a comment

Improve time complexity of removal of callback handle. #11751

Improve time complexity of removal of callback handle. #11751

Conversation

chaoqin-li1123 commented Jun 25, 2020 • edited Loading

repokitteh-read-only bot commented Jun 25, 2020

chaoqin-li1123 commented Jun 27, 2020

stevenzzzz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevenzzzz commented Jun 29, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

htuch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

htuch left a comment

Choose a reason for hiding this comment

chaoqin-li1123 commented Jun 25, 2020 •

edited

Loading